-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cpp-restsdk] add support for oneOf via std::variant #18821
Conversation
|
||
set(CMAKE_CXX_STANDARD 17) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aminya did you need to add the above line to CMakeLists file in order to build the client?
I tried the above but no luck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't need this. The few lines below automatically use the latest standard. This should be removed.
What's the failure that's blocking this PR? |
This reverts commit 0cec8f7.
the test failure details can be found inhttps://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/29877/workflows/eb40bf75-0cfe-4e7d-a276-a6a7b0eb1945/jobs/96564 e.g.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use setup-cpp for setting up the C++ environment
Co-authored-by: Amin Yahyaabadi <[email protected]>
set(CMAKE_CXX_STANDARD 14) | ||
else() | ||
set(CMAKE_CXX_STANDARD 11) | ||
endif() | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some logging to be able to see what is happening. The test would need re-generation.
endif() | |
endif() | |
message(STATUS "Using C++ standard ${CMAKE_CXX_STANDARD}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it's using C++ 11 according to the error message:
In file included from /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/cpp-restsdk/client/include/CppRestPetstoreClient/model/CreateUserOrPet_request.h:39:
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/variant:116:20: error: no template named 'add_const_t'; did you mean '::std::add_const_t'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the libstdc++
version. But the compiler standard itself could be different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the issue! #18821 (comment)
class Category; | ||
class Tag; | ||
|
||
#include <variant> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the problem. The include is being rendered inside the namespace instead of top-level
{{#oneOf}}{{#-first}} | ||
#include <variant> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move the include towards the top so that it is before the namespaces. Probably before or after {{#imports}}
?
…com/openapitools/openapi-generator into revert-18820-revert-18474-cpp-anyOf
Awesome. The tests now pass! |
Reverts #18820
based on #18474